Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow and handle non-parameters in rasterize.instance() #5811

Merged
merged 5 commits into from
Jul 21, 2023
Merged

Allow and handle non-parameters in rasterize.instance() #5811

merged 5 commits into from
Jul 21, 2023

Conversation

ahuang11
Copy link
Collaborator

@ahuang11 ahuang11 commented Jul 14, 2023 •
edited
Loading

I don't fully understand this...

If I use HoloViews alone / try to reproduce with HoloViews:

import numpy as np
import pandas as pd
import holoviews as hv
hv.extension("bokeh")
from holoviews.operation.datashader import rasterize

df = pd.DataFrame(
    np.random.multivariate_normal((0, 0), [[0.1, 0.1], [0.1, 1.0]], (500000,))
)
df.columns = ["a", "b"]

curve = hv.Curve(df, kdims=["a"], vdims=["b"]).opts("Curve", line_width=1)
hv.operation.apply_when(curve, operation=rasterize, predicate=lambda x: len(x) > 1000)

It runs fine...

However, if I use hvplot with this PR:

import numpy as np
import pandas as pd
import hvplot.pandas

df = pd.DataFrame(
    np.random.multivariate_normal((0, 0), [[0.1, 0.1], [0.1, 1.0]], (500000,))
)
df.columns = ["a", "b"]
df.hvplot("a", "b", rasterize=True, aggregation_threshold=1000)

I get the following traceback:

File [~/repos/holoviews/holoviews/operation/datashader.py:1389](https://file+.vscode-resource.vscode-cdn.net/Users/ahuang/repos/sketches/~/repos/holoviews/holoviews/operation/datashader.py:1389), in rasterize._process(self, element, key)
   1387 all_allowed_kws = set()
   1388 all_supplied_kws = set()
-> 1389 instance_params = {
   1390     k: getattr(self, k) for k in self.__instance_params
   1391 }
   1392 for predicate, transform in self._transforms:
   1393     merged_param_values = dict(instance_params, **self.p)

File [~/repos/holoviews/holoviews/operation/datashader.py:1390](https://file+.vscode-resource.vscode-cdn.net/Users/ahuang/repos/sketches/~/repos/holoviews/holoviews/operation/datashader.py:1390), in (.0)
   1387 all_allowed_kws = set()
   1388 all_supplied_kws = set()
   1389 instance_params = {
-> 1390     k: getattr(self, k) for k in self.__instance_params
   1391 }
   1392 for predicate, transform in self._transforms:
   1393     merged_param_values = dict(instance_params, **self.p)

AttributeError: 'rasterize' object has no attribute 'line_width'

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023 •
edited
Loading

Codecov Report

Merging #5811 (ff26cf6) into main (e83355c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5811      +/-   ##
==========================================
+ Coverage   88.17%   88.22%   +0.04%     
==========================================
  Files         307      307              
  Lines       62908    62983      +75     
==========================================
+ Hits        55471    55565      +94     
+ Misses       7437     7418      -19
Flag Coverage Δ
ui-tests 22.39% <19.04%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
holoviews/operation/datashader.py 83.38% <100.00%> (+0.15%) ⬆️
holoviews/tests/operation/test_datashader.py 97.45% <100.00%> (+0.04%) ⬆️

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hoxbro
Copy link
Member

hoxbro commented Jul 15, 2023

This is a MRE:

import holoviews as hv
import numpy as np
import pandas as pd
from holoviews.operation.datashader import rasterize

hv.extension("bokeh")

df = pd.DataFrame(
    np.random.multivariate_normal((0, 0), [[0.1, 0.1], [0.1, 1.0]], (500000,))
)
df.columns = ["a", "b"]

curve = hv.Curve(df, kdims=["a"], vdims=["b"])
inst = rasterize.instance(line_width=2)
hv.operation.apply_when(curve, operation=inst, predicate=lambda x: len(x) > 1000)

__instance_params was added in #5767, and by the look of how it worked before this fix should be okay.

Is it possible to add a small unit test?

hoxbro
hoxbro reviewed Jul 17, 2023
View reviewed changes
Copy link
Member

@hoxbro hoxbro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments, but maybe it is correct behavior to error out here.

When using .instance() and one of the inputs is not a parameter (here line_width), it is not using it. Beforehand this will just silently fail, but with the recent changes, it gives an error, which I'm leaning toward being a good thing.

Take a look at these examples (with your changes / the behavior before #5767):
image

import holoviews as hv
import numpy as np
import pandas as pd
from holoviews.operation.datashader import rasterize

hv.extension("bokeh")
np.random.seed(10)

df = pd.DataFrame(
    np.random.multivariate_normal((0, 0), [[0.1, 0.1], [0.1, 1.0]], (500000,)),
    columns=["a", "b"],
)
curve = hv.Curve(df, kdims=["a"], vdims=["b"])

rasterize.instance(line_width=2)(curve).opts(xlim=(1.1, 1.2), ylim=(1.1, 1.2))
rasterize(curve, line_width=2).opts(xlim=(1.1, 1.2), ylim=(1.1, 1.2))

So maybe in the hvplot code, we should use a partial function for the operation:
hv.operation.apply_when(curve, operation=partial(rasterize, line_width=2), predicate=lambda x: len(x) > 1000)

holoviews/tests/operation/test_datashader.py Show resolved Hide resolved
holoviews/tests/operation/test_datashader.py Show resolved Hide resolved
@hoxbro hoxbro requested a review from philippjfr July 18, 2023 12:10
@hoxbro hoxbro added this to the 1.17.0 milestone Jul 19, 2023
@hoxbro
Copy link
Member

hoxbro commented Jul 20, 2023

I have updated the code to save non-parameter in the instance creation, so it works as one would expect:

image

@philippjfr
Copy link
Member

Looks great, thanks!

@hoxbro hoxbro changed the title Fix rasterizing with apply_when by checking whether object hasattr Handle non-parameters in rasterize.instance() Jul 20, 2023
@hoxbro hoxbro changed the title Handle non-parameters in rasterize.instance() Allow and handle non-parameters in rasterize.instance() Jul 20, 2023
@hoxbro hoxbro merged commit 8d6676c into main Jul 21, 2023
@hoxbro hoxbro deleted the fix_datashade_line_with_apply_when branch July 21, 2023 06:44
@github-actions GitHub Actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hoxbro hoxbro hoxbro left review comments

@philippjfr philippjfr Awaiting requested review from philippjfr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ahuang11 @codecov-commenter @hoxbro @philippjfr